Skip to content

Feature/enable ctypes#343

Merged
tito merged 6 commits into
kivy:masterfrom
olymk2:feature/enable-ctypes
May 10, 2015
Merged

Feature/enable ctypes#343
tito merged 6 commits into
kivy:masterfrom
olymk2:feature/enable-ctypes

Conversation

@olymk2

@olymk2 olymk2 commented Mar 20, 2015

Copy link
Copy Markdown
Contributor

Seems to be working can someone else check and merge if happy ?

@GeoRW

GeoRW commented Apr 4, 2015

Copy link
Copy Markdown

tried it and all works fine in my environment. Ctypes compiled, packaged and runs on Android as expected. Cheers

@olymk2

olymk2 commented Apr 5, 2015

Copy link
Copy Markdown
Contributor Author

cheers for testing that @GeoRW hopefully someone will merge it soon

@olymk2

olymk2 commented Apr 25, 2015

Copy link
Copy Markdown
Contributor Author

Bump for review / merge

@hradec

hradec commented May 7, 2015

Copy link
Copy Markdown

just a heads up. I've tried this pull request, and noticed that PIL's _imaging.so file is not building correctly.
distribute.sh builds a 0Kb _imaging.so file, without giving any errors or telling anything.

I'm investigating it further since I need ctypes for a project of mine, so I'll let you guys known if I find anything. But from an initial investigation, it could be related to the python/recipe.sh change, which explicit specifies the BUILDARCH as x86 linux, where before it didn't.

btw, to re-create the error, you have to clean clone a HEAD of p4a, and then apply this pull before running distribute.sh, so to avoid building PIL _imaging.so without it which builds fine!!

@olymk2

olymk2 commented May 7, 2015

Copy link
Copy Markdown
Contributor Author

@hradec thanks for testing and reporting back I will have a look into this when I get a chance as well see if I can figure out anything.

@hradec

hradec commented May 7, 2015

Copy link
Copy Markdown

Ok... after lots of tests, it seems _imaging.so is always 0Kb, even without this patch and everything seems to work ok.

Also, there are no errors when running my app with ctypes and this patch, if I don't call my custom built .so shared library function using ctypes.

Just importing ctypes in my python script also doesn't throw any error, so I'm assuming the problem is inside my shared library.

the error message is a bit misleading though, as you can see:

AttributeError: Symbol not found: load_library(linker.cpp:745): library "/data/data/org.test.touchtracer/files/lib/python2.7/lib-dynload/_imaging.so" not found

but that error only happens when I call my function from my custom shared library with ctypes.

so, I think it's probably my problem, not the patch!

sorry about this.
cheers...

@kived

kived commented May 7, 2015

Copy link
Copy Markdown
Contributor

@hradec @olymk2 This is expected. Android has a limit on the number of libraries you can access via dlopen() in versions prior to 4.3, so instead of generating individual .so files for each module we combine them together in libpymodules.so. A zero byte file is put in place of the original .so as a placeholder.

@hradec

hradec commented May 7, 2015

Copy link
Copy Markdown

haaaa... that's it then!!! good to known! Thanks!!

@olymk2

olymk2 commented May 7, 2015

Copy link
Copy Markdown
Contributor Author

@hradec also worth noting that ctypes does by default does not detect where the libraries are because it trys to detect your architecture and fails on android so i had to patch it to look in a folder called /lib with in your app dir which i think is where they normally get put.

if your dealing with your own code worth bearing in mind if you look in the PR you can see the logic at least it looks some where instead of no where now :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a good way to do this. It works right now, while we use Python 2.7.2. Python 2.7.3+ will always report linux2 regardless of the Linux version, and Python 3 dropped the number entirely. You should just replace the original function rather than adding extra checks, since we want them to always evaluate to True anyway. :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay i was not that keen on dropping that much code, as there are a lot of conditions but perhaps its okay just will make the patch a lot bigger.
Will see if i can get this done at the weekend

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kived I have created a new patch removing the system checks and just leaving the find_library function as suggested.

Can some people test and let us know if it works and perhaps help get it merged :)

@hradec

hradec commented May 7, 2015

Copy link
Copy Markdown

humm... what I did was to add my own private folder, and put my .so in
there.

I was leaving it with the python code, but since pyo files go to sdcard,
which is not executable, ctypes was failing to load it!

putting it in private seems to have done the job for me, since the files
end up in the same place as _ctypes.so, which seems to be in the shared
library search path! 👍😉

On Thursday, May 7, 2015, Oliver Marks notifications@github.com wrote:

@hradec https://github.com/hradec also worth noting that ctypes does by
default does not detect where the libraries are because it trys to detect
your architecture and fails on android so i had to patch it to look in a
folder called /lib with in your app dir which i think is where they
normally get put.

if your dealing with your own code worth bearing in mind if you look in
the PR you can see the logic at least it looks some where instead of no
where now :)


Reply to this email directly or view it on GitHub
#343 (comment)
.

… assumes android and runs our custom library lookup function
@tito

tito commented May 10, 2015

Copy link
Copy Markdown
Member

Looks almost good to me. I wonder if we can make it optionnal, or at least don't fail if we delete the _ctypes.so in the final apk (it is an additionnal module not required for most of the app). Could you change to add a try/catch around it?

I don't really like the patch if the find_platform(), all the deletion stuff will break when upgrading a new python version (VS moving the os.name test for linux with "and False" or rewrite it, and add a new test at the end = patch easier to read / maintain)

Double-check your PR, there is few part where the indentation is not working ^^

@olymk2

olymk2 commented May 10, 2015

Copy link
Copy Markdown
Contributor Author

happy to make the changes but you may need to be more specific.

@tito and @kived currently if _ctypes.so is missing it just errors if you try and import ctypes, not sure where i would add the try except do you mean around find library ? thats just for loading other .so files and not related to ctypes.so

I could put the other code back in for the platform tests and perhaps just make the first test if posix use custom ctypes find_library else try all the other rules ?

with the indentation do you mean outside the python files ? i did test the python code so would have hit an indentation error, i will have a look but can we clarify the other points before i do the work.

its a pain each time i do a change as its about an hour of building to test a new package as you have to completely clean the build folder to test properly :/

@tito

tito commented May 10, 2015

Copy link
Copy Markdown
Member

I was talking in the java part, dunno why, but you added a _ctypes.so to load. Because he doesn' thave its own try/catch, it might break for later load? (nothing to worry about the python side). Maybe i don't understand and in that case, you're all good ^^

Just look the PR at the end, there is indentation changes on your own line. Maybe tab/space mix up

@olymk2

olymk2 commented May 10, 2015

Copy link
Copy Markdown
Contributor Author

@tito can you check the changes i have made if it looks good i will run a test and squash the commits.

@tito

tito commented May 10, 2015

Copy link
Copy Markdown
Member

@olymk2 YES! one last question, did you try your new version, aka on the java part, is both load are necessary to make it work? The previous one was having only the System.load() not the loadLibrary()

@olymk2

olymk2 commented May 10, 2015

Copy link
Copy Markdown
Contributor Author

@tito I have not tried it yet wanted to make sure it looked good from code point of view in case more changes are needed, i will run it now and report back.

I did have both load and loadlibrary previously olymk2@e8c0a4e#diff-3f3340c82c7f6df0d6c292b442086e1bL88

@tito

tito commented May 10, 2015

Copy link
Copy Markdown
Member

ok, my bad :)

Well done @olymk2 !

tito added a commit that referenced this pull request May 10, 2015
@tito tito merged commit a36dc6e into kivy:master May 10, 2015
@olymk2

olymk2 commented May 10, 2015

Copy link
Copy Markdown
Contributor Author

no problem, um i think you missed the part in my above two comments about me not having tested the latest changes. the build has finished anyway and my app still works doing a fresh build so no issues :)

cheers for taking the time and having a look i have been keen to get this merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants